Repository navigation
Empty string if out of range - #65
droppingin wants to merge 5 commits into
Conversation
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
=======================================
Coverage 66.23% 66.23%
=======================================
Files 4 4
Lines 465 465
=======================================
Hits 308 308
Misses 146 146
Partials 11 11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
All of these points are correct, but I have to confess the "Day(451)" values have always bothered me, I feel like that kinda change might break more people than we might expect. I think we might be stuck with this, I'm not saying no, just need to really think about it more. |
error will be passed through all the converters
4919e1c to
b954aa6
Compare
|
Thank you, and sorry this sat so long. String() returning "Day(451)" is the stringer contract and I'm keeping it. The idea that Value() and the marshalers should error on invalid values is a fair one, but it would need to cover JSON, text and GQL too, and as written the string() helper is missing from the multiple-runs template so -sql or -yaml on those types doesn't compile. |
First of all, @dmarkham, thank you for the wonderful library.
We are happily use it in our go project.
I will go straight to the case.
When the receiver does not support the new enum value, it accepts it formally.
Next, the string representation returns a formatted value, like "Day(451)".
I allowed myself to return an empty string in case of errors.
I understand that it reduces verbosity but i think it matches better the go principles.
The other thing is error handling. Some converters always return nil error despite of the fact that the value is incorrect.
I have took it into account and implemented it as follows:
As you can see, the superposition of error handing e.g. in SQL marshaller and formatting of unknown value can lead to funny behavior.
In our case, there was a bug in our code related to the 2 described edge-cases.
Now i present the PR to you hoping you find some time to look at it and tell your opinion.
Thank you for your attention!